Align the semantics of http.request.size with http.response.size#7100
Align the semantics of http.request.size with http.response.size#7100qdongxu wants to merge 2 commits intocaddyserver:masterfrom
Conversation
|
There are 2 things that are unclear to me: |
As for the import, it shocks me too🤣. It was mostly generated by the goland IDE. I am sorry I didn't check formattings before checking in. As for the double pointer, the http.Request.WithContext works in a immutable way, it should returns a new instance when change the context. It looks cohensive to make the change inside the c := context.WithValue(r.Context(), ResponseRecorderVarKey, rr)
*req = r.WithContext(c)A alternative way is to let the invoker to take care of the new r . The side effect is not obvious for the invoker(The input r changed??) func NewResponseRecorder(w http.ResponseWriter, req *http.Request, buf *bytes.Buffer,
shouldBuffer ShouldBufferFunc) (wrec ResponseRecorder, newReq *http.Request , cached bool)
// ....
wrec, r, cached := NewResponseRecorder(w, r, nil, nil)It's weird to input a double pointer but it's may get the invoker's attention as I had thought. What's your opinion on the alternatives ? |
|
I converted this PR to draft. During review the caching of |
2. make metrics records accurate reqeust size by read from req.Body. ( the current metrics read from Content-Length field, which not applicable to websocket or chunked Body)
I have to admit it's complicate than I had expencted 🤣. It's better to make this PR scope smaller and focus on the issue of adding |
|
I will submit it for a later time. |
This PR is for Issue #7087
cache RequestRecorder to handle records in multiple modules. ( logRequest and metrics creates its own ResponseRecorder.)lengthReaderas a member ofResponseRecorder. it takes quite a time to understand when they separated and share a commonintpointer.